Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PyCapsule API #1980

Merged
merged 36 commits into from
Nov 23, 2021
Merged

Add PyCapsule API #1980

merged 36 commits into from
Nov 23, 2021

Conversation

milesgranger
Copy link
Contributor

@milesgranger milesgranger commented Nov 10, 2021

Will close #1193

Ref: https://docs.python.org/3/c-api/capsule.html#capsules

TODO:

  • an entry in CHANGELOG.md
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

@davidhewitt
Copy link
Member

Just wanted to say thank-you for working on this - it's a really nice feature to add to PyO3. I've been a bit busy recently, however trying to find a moment to review when I can...

@milesgranger
Copy link
Contributor Author

No worries; thank you! I'm still chipping away as I get some time here and there. I'm sure there are some glaring errors I've missed, so put it here anyway if someone wanted to take a peak.

@mejrs
Copy link
Member

mejrs commented Nov 12, 2021

Thanks! Sorry for not getting to this sooner.

Unfortunately casting away lifetimes and types like this is really, really unsafe.

For example, this segfaults:

fn main() {
    let cap: Py<PyCapsule> = Python::with_gil(|py| {
        let name = CString::new("foo").unwrap();
        let cap = PyCapsule::new(py, (), &name, None).unwrap();

        let stuff: Vec<u8> = vec![1, 2, 3, 4];

        cap.set_context(py, &stuff).unwrap();

        cap.into()
    });

    Python::with_gil(|py| {
        let ctx: Option<&&Vec<u8>> = cap.as_ref(py).get_context(py).unwrap();
        dbg!(ctx);
    })
}

Or, you can implement transmute:

fn transmute<'p, T, U>(py: Python<'p>, t: T) -> &'p U {
    let name = CString::new("foo").unwrap();
    let cap = PyCapsule::new(py, (), &name, None).unwrap();
    cap.set_context(py, &t).unwrap();
    let ctx: Option<&U> = cap.get_context(py).unwrap();
    ctx.unwrap()
}

fn main() {
    Python::with_gil(|py| {
        let foo = 32u8;
        let boom: &Vec<u32> = transmute(py, &foo);
        dbg!(boom);
    });
}

And you can also use Py<PyCapsule> to smuggle things like Rc to other threads.

A good way to fix some of this is to introduce some variance - i.e., make the type PyCapsule<T>, so that type information is not lost. However this may make a safe api of PyCapsule almost useless.

Maybe you can use dyn Any for this? You can do some runtime reflection with it.

@milesgranger
Copy link
Contributor Author

I suspected I'd fouled that up a bit. Thanks for the thorough explanation, the failing example is nice to have. I'll work on it; suspect this PR will be "slow to update". Working on it as time presents itself. :-)

@milesgranger
Copy link
Contributor Author

milesgranger commented Nov 13, 2021

My current understanding is capsules are used to store by value and not by reference (unless static reference). If I modify your example to pass by value, it works fine. Thus, if I update the signatures of new and set_context to have T: 'static then at least your example would no longer compile. Then current tests and the updated example here all pass:

pub fn new<'py, T: 'static>(
        py: Python<'py>,
        value: T,
        name: &CStr,
        destructor: Option<ffi::PyCapsule_Destructor>,
    ) -> PyResult<&'py Self>;

pub fn set_context<'py, T: 'static>(&self, py: Python<'py>, context: T) -> PyResult<()>;

I'm still pretty confident the value of Box::into_raw(T) as *mut c_void leaked. Which I suppose is another issue.

Edit:
Nvm, using heaptrack and some bloated test Vecs, it doesn't seem like it's leaked. Assume Python ends up owning the pointer/data?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, in addition to discussion above I finally had some time to give this a read today. I think there's a few things which need to be solved yet.

src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

The pointer value of the capsule is pointing to a struct for T and F, and somehow get_reference(&self) -> &T still works, even though it's really just getting the pointer to this CapsuleContents struct that holds the value and destructor.

Because the first field of the CapsuleContents struct is the T, through blind luck the compiler's put that at the front of the struct and so the cast works. This is UB because the default struct layout is not guaranteed. If you add #[repr(C)] to CapsuleContents (maybe with a comment why that repr is used), it's sound to rely on first field T always being at zero offset inside the struct.

https://doc.rust-lang.org/reference/type-layout.html#the-c-representation

Do you think having a macro of some sort for a destructor would be helpful for the user?

It's not clear to me what the use case for this would be. Also, macros are hard for users to figure out how to use without lots of good documentation, so without a strong motivation I'm unconvinced it's worth the maintenance cost.

src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

Thanks for the continued progress on this, I think it's getting there and will be a nice feature to add to PyO3!

@milesgranger
Copy link
Contributor Author

milesgranger commented Nov 19, 2021

Thank you for the thorough reviews here, a lot of new stuff here for me, so appreciate it. I know you're busy! 🙏

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is looking great to me! Just needs an Added CHANGELOG entry and then a couple of final suggestions. Also it might be nice to squash the commits, though I can also do that on merge.

Thanks for implementing this.

src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

👍 this is shaping up really well, just wanted to say thanks again for working on this. It's a tricky one with a few design gotchas which are taking a bit of effort to hash out, so the patience with repeated iterations is appreciated. I think with luck we're close now 🤞

Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few nits left :)

src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
src/pycapsule.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

@birkenfeld as I think you're very much on top of this PR, do you want to approve, squash-and-merge it once you're happy? Thanks for comprehensive reviewing, and thanks @milesgranger for all the iterations put in! 35 commits 😮

@milesgranger
Copy link
Contributor Author

It's been a pleasure, learned a lot, and really appreciate the repeated and helpful reviews! 🙏

@birkenfeld
Copy link
Member

@davidhewitt will do - should the module be renamed to capsule though (to be consistent with buffer which is also a CPython type, as opposed to pycell)?

@milesgranger I'll do this change if @davidhewitt agrees and then merge it, everything else LGTM!

@davidhewitt
Copy link
Member

davidhewitt commented Nov 22, 2021

Ah yes good spot, it should probably even live in src/types/capsule.rs and be exposed as pyo3::types::PyCapsule ?

@birkenfeld
Copy link
Member

That's probably even better. Not sure why buffer is so lonely at toplevel :D

@davidhewitt
Copy link
Member

PyBuffer is this weird thing that isn't really a Python object, so it's kind of lost wandering the wasteland of murky design 😅

and make slight copyedits to the docstring.
@birkenfeld
Copy link
Member

Thanks again @milesgranger!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose PyCapsule API
5 participants